Skip to content

lukas/websocket improvements #1807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

LukasDeco
Copy link
Collaborator

@LukasDeco LukasDeco commented Aug 8, 2025

Overview:

  • This PR introduces a new version of the websocket account subscriber for the drift client. (WebSocketDriftClientAccountSubscriberV2)
  • This uses WebSocketProgramAccountSubscriberV2 under the hood which opens a single programAccountSubscribe WS connection for an account type.
  • We open a connection for PerpMarkets and a second connection for SpotMarkets, and if we detect that a certain market account isn't getting an update that it should, we refresh the connection. This is done through polling logic rather than assuming things are stale and refreshing the connection, which is more resource intensive than just doing a poll.
  • We should have a faster subscribe time on initial load, and less websocket connections, but still have the preferable benefit of real-time events that we don't get with polling.
  • These classes leverage the v2 of solana's web3.js known was kit or gill depending on which npm you install. We are using gill here. There are some inherit websocket optimizations for memory management and stability.

Notes:

  • There is some initial code added for WebSocketProgramUserAccountSubscriber but that is not going to be utilized immediately on the UI. But it is added and ready to be used when we want.
  • Please review the code for readability, maintainability and any performance optimizations that might be available to add.
  • This PR should NOT affect any current consumer of the SDK UNLESS they specifically opt-in to these new classes via the config.

Commits:

  • feat: initial implementation for users and markets WS improvements
  • lukas/gill websocket sub (lukas/gill websocket sub #1781)
  • feat: initial implementation for users and markets WS improvements
  • feat: polling check on websocket acct subscriber v2 + naming
  • fix: lint
  • fix: non-hanging WS subscription async loop handling
  • fix: bugs with program ws subs hanging on asynciter
  • fix: goofy self imports
  • feat: initial batch fetching temp
  • temp: sub second WS subscribe time
  • fix: ws program account subscriber v2 bugs and optimizations

@LukasDeco LukasDeco marked this pull request as ready for review August 12, 2025 17:05

**How it works:**
1. Initially subscribes to WebSocket updates
2. If no WebSocket data is received for `resubTimeoutMs` (30s), switches to polling mode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only true, if usePollingInsteadOfResub is true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your right

// Start notification loop without awaiting
this.handleNotificationLoop(subscription);
// Start notification loop with the subscription promise
this.handleNotificationLoop(subscriptionPromise);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels weird to be accepting a promise for a function

maybe could use .then instead and pass the resolved Promise to this.handleNotificationLoop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok it is totally weird but I did this for performance haha. I didn't want to wait for the WS subscription to succeed in the overall .subscribe() path because it slows down the app from rendering, while we can put this off to a separate thread because we do an initial fetch of the data anyway.

this.subscriptionPromiseResolver(true);

// delete initial data
this.removeInitialData();
Copy link
Collaborator

@ChesterSim ChesterSim Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be here? this.initialPerpMarketAccountData and this.initialSpotMarketAccountData are set just above, but we are deleting it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk tbh 😄 I was just following the original pattern from v1 which does this as well. Afraid to remove it and cause a regression. WDYT should we try removing it?

/**
* This is a no-op method that always returns true.
* Unlike the previous implementation, we don't need to manually subscribe to individual perp markets
* because we automatically receive updates for all program account changes via a single websocket subscription.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something that im unsure - when you mention single websocket subscription for all program account changes, this means a single WS subscription for all perp market account changes right? both spot markets and perp markets use a different websocket?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could technically use a single websocket for both perp and spot markets. But in this case basically yeah we use one WS connection for all perp market account changes. I think it might be fine to keep them separate. There are benefits to keep them separate like isolation if one of the connections has a problem, it doesn't affect state for the other states.


await this.unsubscribeFromMarketAccounts();
await this.unsubscribeFromSpotMarketAccounts();
await this.unsubscribeFromOracles();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these use Promise.all()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call

@@ -51,6 +71,16 @@ export class WebSocketProgramAccountSubscriberV2<T>
private accountsCurrentlyPolling: Set<string> = new Set(); // Track which accounts are being polled
private batchPollingTimeout?: ReturnType<typeof setTimeout>; // Single timeout for batch polling

// For batching initial account fetches
private accountsPendingInitialMonitorFetch: Set<string> = new Set(); // Track accounts waiting for initial monitor fetch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without much docs on this, it really confused me a lot, thinking why we are doing the fallback polling at the beginning

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure let me add docs for this functionality

@ChesterSim
Copy link
Collaborator

what if resubOpts isn't passed in?

Screenshot 2025-08-15 at 5 59 04 PM

@ChesterSim
Copy link
Collaborator

i think the idea is there, but its been pretty hard to review ngl...

I don't think its a you problem btw, this whole SDK has been coded this way where each class contains too much logic. One suggestion is to ask Cursor to help abstract this class' logic into utility managers e.g. WebsocketSubscriberManager, FallbackPollingManager, InitialFallbackPollingManager etc.

@LukasDeco LukasDeco force-pushed the lukas/websocket-improvements branch from 0388104 to 7195770 Compare August 15, 2025 18:17
@LukasDeco
Copy link
Collaborator Author

i think the idea is there, but its been pretty hard to review ngl...

I don't think its a you problem btw, this whole SDK has been coded this way where each class contains too much logic. One suggestion is to ask Cursor to help abstract this class' logic into utility managers e.g. WebsocketSubscriberManager, FallbackPollingManager, InitialFallbackPollingManager etc.

I appreciate that and I think you are right. I've added some deeper docs on the WebsocketProgramAccountSubscriberV2 and WebsocketAccountSubscriberV2 which I think need it the most so the polling vs ws logic in there is understood.

Regarding adding utility manager classes, I definitely think some smaller classes/better code isolation and single responsibility could be very helpful. I am wondering if maybe we could start with adding real good docs on these classes, and then I could do a follow up to break down logic into shareable utility classes/functions. Since that would be a huge refactor and then potentially implicates adjusting other classes so I'd wanna make sure I did it real well. Although I could just give it a short in this PR. Up to you.

Thanks so much for the review!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants